Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Quick'n'dirty CLI for the light client#5002

Merged
rphmeier merged 96 commits into
masterfrom
lightcli
Apr 5, 2017
Merged

Quick'n'dirty CLI for the light client#5002
rphmeier merged 96 commits into
masterfrom
lightcli

Conversation

@rphmeier
Copy link
Copy Markdown
Contributor

@rphmeier rphmeier commented Mar 22, 2017

Summary of changes:

  • Use database in the light client (particularly relevant to header_chain.rs)
  • Add column to the database specifically for the light client.
  • Add execute_light function to parity/run.rs, triggered on --light flag.
    Runs RPC servers, but no DApps yet: Make Dapps server light-client friendly #5001
  • Transaction propagation and transaction queue culling.
  • Various minor enhancements to the dispatch/request credits mechanism
  • Fixing some message encoding issues
  • Serve light clients by default, opt out with --no-serve-light.

Next needed changes:

  • Integration with main informant.
  • Filters and other missing RPCs needed by the UI
  • Better dispatch mechanism for on_demand
  • Better defaults for request credits: 10 headers/sec/peer is too slow if there's only one peer!

Closes #5012

@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Mar 23, 2017
Comment thread parity/cli/usage.txt Outdated
--identity NAME Specify your node's name. (default: {flag_identity})
--light Experimental: run in light client mode. Light clients
synchronize a bare minimum of data and fetch necessary
data on-demand from the network. Much lower in storage,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces should be used here to guarantee output alignment.

Comment thread parity/cli/usage.txt Outdated
synchronize a bare minimum of data and fetch necessary
data on-demand from the network. Much lower in storage,
potentially higher in bandwidth. Has no effect with
subcommands (default: {flag_light})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget the full-stop ..

@gavofyork
Copy link
Copy Markdown
Contributor

is this really 1,522 −388 ? seems a lot for the documented additional functionality...

@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Apr 3, 2017

It's about +450 for the header chain stuff, +150 for the transaction propagation, +700 for the CLI (generalizing RPC APIs among other things), and then about +200 for miscellania to make the UI function a bit better.

}

fn cht_key(number: u64) -> String {
format!("canonical_{}", number)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Key comparison should work faster if you put the number in the front. Might be also a good idea to use a 4-byte big endian representation instead, so that enumeration would produce a sorted list.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the keys compared during regular lookup? Also, is there a minimum length on keys?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the keys compared during regular lookup?

Pretty sure they are. Database basically performs a binary search.

Also, is there a minimum length on keys?

I believe there is no such minimum.

};

self.headers.read().get(&hash).cloned()
load_from_db(hash)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth caching the best header in memory.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DB should do this automatically, right? I plan to add customized caching in the future but I think it's beyond the scope of this PR.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy if this is added later. The closer is the cache to the code that uses it, the better is performance. Introducing multiple levels of caching can bring huge benefits. Higher level cache usually holds less data but is accessed much more frequently. CPU hardware design generally follows this principle and it often applies to the software as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logged in #5398

@arkpar arkpar added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 4, 2017
@rphmeier rphmeier added A0-pleasereview 🤓 Pull request needs code review. and removed A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. labels Apr 5, 2017
@rphmeier
Copy link
Copy Markdown
Contributor Author

rphmeier commented Apr 5, 2017

@arkpar headerchain grumbles addressed

@arkpar arkpar added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Apr 5, 2017
@rphmeier rphmeier merged commit 8486e79 into master Apr 5, 2017
@5chdn 5chdn deleted the lightcli branch September 21, 2017 09:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants